Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import & Run #86

Draft
wants to merge 4 commits into
base: trunk
Choose a base branch
from
Draft

Import & Run #86

wants to merge 4 commits into from

Conversation

dainemawer
Copy link
Contributor

@dainemawer dainemawer commented Feb 28, 2022

Description of the Change

Fixes #80

Alternate Designs

Possible Drawbacks

Verification Process

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Changelog Entry

Credits

Props @

@dainemawer dainemawer changed the title Fixes #80 - Import & Run Import & Run Feb 28, 2022
@dainemawer
Copy link
Contributor Author

@rdimascio - I noticed a strange bug here. The navigation component should not run unless the selectors array matches, but it seems to run regardless. Do you mind taking a look?

@rdimascio
Copy link
Contributor

Thanks for putting this together @dainemawer! I fixed the bug you mentioned, cleaned up the architecture a bit, added error handling and some documentation. This is looking good to me.

cc @joesnellpdx @devinle @nicholasio @fabiankaegy

@devinle
Copy link

devinle commented Mar 2, 2022

@rdimascio @dainemawer this is looking good to me. Nice and clean - easy to follow. I also like the ability to pass a custom callback. Well done!

@joesnellpdx
Copy link
Contributor

Per discussion with @rdimascio ...

Is this something we want to include in 90% of our projects, or are there particular use cases that would trigger the usage of import-and-run?

If it is not something we want to have as default, do we consider one of the following?:

  • Add as an enhancements section in the README - with guidance / examples
  • Add a new enhancements doc - link to it in the README - and include guidance / examples for this
  • Add an examples directory in the project that is not included with references in the README file / comments
  • Add to existing architecture, but comment out with comment guidance when/ how to activate (and add to README)

Either way, there needs to be more clear guidance and documentation for engineers if this is implemented.

@devinle @dainemawer

Copy link
Member

@nicholasio nicholasio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are we thinking in terms of recommendations for this?

Are we going to essentially code split every single front-end component?

I do not think code-splitting every single front-end component is worth it. There's an overhead of making an additional call to load the JS. If we ever ship something like this I feel we need more guidance around when to use it. While I do not have any benchmark to back me up here I do not think code-splitting very small components would actually give any meaningful improvement, while potentially making things a little worse. We should also be mindful of CLS, if every single front-end component is lazy-loaded, this might mean we're delaying fully rendering stuff on the front end which could cause content shifting

@@ -0,0 +1,5 @@
export default class Navigation {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually need to be a class?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not, but I think the large majority of our projects are still using class based components. I added a comment for sites with functional components.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicholasio this was just added for example sake - as long as this component is the default export we should be okay. Happy to update it to a const Navigation = () => {}

@rdimascio
Copy link
Contributor

@nicholasio I would think that script parse and execution time on page load would result in a worse user experience and CWV scores than network requests. I don't have any benchmarks for this either (and @dainemawer can probably provide more insight here) but I believe the implementation for this approach (when and even how to use it) will larger depend on project architecture. I'm in agreement that there needs to be more clear guidance and documentation on this.

@nicholasio
Copy link
Member

xecution time on page load would result in a worse user experience and CWV scores than network requests. I don't have any benchmarks for this either (and @dainemawer can probably provide more insight here) but I believe the implementation for this approach (when and even how to use it) will larger depend on project architecture. I'm in agreement that there needs to be more clear guidance and documentation on this.

You're right but it depends on what you're code splitting. Every optimization comes with an overhead cost. Finding the balance is what we're missing here. We're all on the same page we need more documentation and guidance here 👍

@dainemawer
Copy link
Contributor Author

What are we thinking in terms of recommendations for this?

Are we going to essentially code split every single front-end component?

I do not think code-splitting every single front-end component is worth it. There's an overhead of making an additional call to load the JS. If we ever ship something like this I feel we need more guidance around when to use it. While I do not have any benchmark to back me up here I do not think code-splitting very small components would actually give any meaningful improvement, while potentially making things a little worse. We should also be mindful of CLS, if every single front-end component is lazy-loaded, this might mean we're delaying fully rendering stuff on the front end which could cause content shifting

@nicholasio - yeah I agree here, I don't think the intention is to code-split every component. What this PR lacks, as @joesnellpdx mentioned above, is documentation is going forward. The idea here should be to code-split out:

  • Any resource heavy functions / components - potentially components requiring third-party libs like lodash, moment etc.
  • Any scripts that do not need to be loaded in the bundle on initial load - chat libs, cookie banners etc.

I will add some guidance here. I'll also clean up this entry file so that its more instructional.

@dainemawer
Copy link
Contributor Author

In general, benchmarks are a bit all over the place. They are specific to the nature of the site as well. The overall strategy here is to offload any components and scripts that are not absolutely needed for a good user experience on initial load - or above the fold.

An example of this would be something along the lines of:

  1. We may write a component in JS for the Navigation. That JS should not be lazy-loaded. It's imperative for good UX on initial load.
  2. A cookie banner though, whose presence is not in the flow of the document, and does not contribute to important user experience.

Doing this above, and identifying elements with the above nature, means we can keep the byte size of our bundle low.

Copy link

@tylercherpak tylercherpak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dainemawer I love this and think it has a ton of potential to be part of how we think about components and if they are needed at time of initial paint.

One question I want to raise for everyone, do we think we should have an example in which making the component dynamic is more necessary. Such as a slider that needs to include additional libraries that we don't want to load at render or if that component isn't present on the page.

Let me know what you think! Thanks again for this!! 😄

},
"config": {
"allow-plugins": {
"dealerdirect/phpcodesniffer-composer-installer": true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dainemawer is this directly related to the dynamic imports?

export const initComponents = () => {
Object.entries(COMPONENTS).forEach(([component, args]) => {
importAndRun(
`frontend/components/${component}`,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we importing and running the un-minified js source? Should webpack be creating a separate bundle for this component?

@fabiankaegy fabiankaegy marked this pull request as draft February 20, 2024 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add importAndRun function for Code Splitting
6 participants